Add action to run benchmarks - #1509
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1509 +/- ##
=======================================
Coverage 90.31% 90.31%
=======================================
Files 60 60
Lines 8663 8663
Branches 8663 8663
=======================================
Hits 7824 7824
Misses 525 525
Partials 314 314 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds benchmark execution and reporting to the existing CI workflow to address #1453 by running cargo bench in GitHub Actions and publishing benchmark results/alerts.
Changes:
- Added a new
benchmarkjob to runcargo benchonubuntu-latest. - Integrated
benchmark-action/github-action-benchmark@v1to publish a job summary and comment on regressions.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
dc2917
left a comment
dc2917
left a comment
There was a problem hiding this comment.
Looks good, and seems logical, just wondering if it's not cleaner to stick the python code in a .py file to run as a script
| - name: Convert Criterion results to JSON | ||
| shell: python3 {0} | ||
| run: | | ||
| import json |
There was a problem hiding this comment.
Might it be tidier to stick this in a .py file?
There was a problem hiding this comment.
Yep. I did not do it because we do not have python code in the repo and did not wanted to pollute things, but it will be tidier. I'll change it.
tsmbland
left a comment
tsmbland
left a comment
There was a problem hiding this comment.
This seems reasonable. Curious how it works in practice:
- If I make a series of pushes to a PR branch, each time it will compare against the last push, not against main, right?
- If you make a single push to a PR branch, will this compare against main or against nothing?
- If, for example, one push regressed by 20% and the next by another 20%, the benchmarks will never fail, even though performance has regressed by more than 30% overall. Right?
- If a series of commits are pushed together, does the alert comment get added to the latest of those commits (even though it could have been an earlier commit that caused the regression)? Or is it not tied to a specific commit?
- In most cases, we'll only know how much a PR has actually regressed against main after merging into main (provided the last merge to main wasn't more than 7 days ago)?
I'd also be tempted just to run the full benchmarking on pushes to main, and include in the testing suite a very small test just to make sure the benchmarks can compile/run without error.
Also, I'm not sure we need to run the full scaling for the assets benchmark, especially because the github summary doesn't give us a nice way to visualise this (it's not even in numerical order!). Would it be sufficient just to run the highest n (20) for sequential and parallel, and exclude 1-19, since we're mostly concerned about regression here rather than scaling?
|
I totally agree with your comments, @tsmbland . The solution would be to compare not with the results of the previous run - which has lots of problems -, but with a baseline. This could be included in the repository, the same way we include the expected results of some tests, and just tell the action to use it as the data to compare with. Now, the problem is how we calculate that baseline, to start with, and how we update it. To be meaningful for this action, it cannot be generated in our laptops - typically more powerful than the runners - but by the Ubuntu runner itself. And even if we have it, it is not clear to me when/how it should be updated with a new version. I guess we could update it automatically with every push to |
|
I've made some changes that I think address most of the comments above. Please, let me know what you think. |
tsmbland
left a comment
tsmbland
left a comment
There was a problem hiding this comment.
Happy to give this a try and see how useful it is in practice. If it's not useful or becomes annoying then we can remove/modify it later.
I can't see benchmark results for the latest push, but presumably that's because there's no saved benchmark-results.json yet to compare against
That's correct. Once we have one baseline, it should hopefully work fine... |
|
Of course, branch protecion rules prevent the commit adding the baseline... |
Description
The build and test action has been updated to run the benchmarks and publish the summary in the action itself, as well as creating an Alert comment in the relevant places wherever there has been a regression.
As we know, the output produced by Criterion is not the standard that the action expects, so we (my friend Claude and myself) embedded in the action a small python script to adapt the output.
The benchmark result, used to compared with future runs, is stored in the GitHub cache, which only last 7 days. If we want to keep it for longer, then we would need to find somewhere else to save it.
A summary of the benchmark is included in the Summary section of the workflow, regardless of it being successful or not.
Fixes #1453
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks